samorev Code Review Report

• PR: fix(ts): back off on nack/ack failure in consumer #289 - fix(ts): back off on nack/ack failure in consumer
• Author: @NikolayS
• AI-Assisted: Unknown

BLOCKING ISSUES (1)

HIGH MR/PR state - Review target is draft

The review target is still marked as draft.
Fix: Mark it ready for review before merge.

Summary

Note:

• Findings: High-confidence issues (8-10/10) - blocking or non-blocking per severity
• Potential: Medium-confidence issues (4-7/10) - review manually
• Filtered: Low-confidence issues (0-3/10) - excluded as likely false positives

provider=github
kind=pr
project=NikolayS/PgQue
number=289
target=github:NikolayS/PgQue#289
state=OPEN
draft=true
diff_lines=187
diff_added=125
diff_removed=6
diff_bytes=7093
comments_count=0
commits_count=2
ci_status=success
ci_summary=total=14 success=14 failure=0 pending=0 other=0
prompt=.claude/commands/review-mr.md
blocking=false
posted_by=gh
no_comment=false
live_posting=posted

samorev-assisted review (AI analysis by Tanya301/samorev)

Deep review + real testing (0.3.0-alpha1 readiness)

Reviewed as unreviewed (the earlier "samorev" pass was disregarded). Rebased onto current main (stable/devel split), verified CI, deep-reviewed the fix, and ran the suite against a fresh devel build.

Rebase

Branch was based on pre-split main and its CI still installed the TS test DB from sql/pgque.sql. Rebased onto origin/main (clean — PR is clients/typescript/ only); CI now installs from devel/sql/pgque.sql and the new stable install smoke (frozen sql/) job runs. Head 1f59900.

CI

All 16 checks green on 1f59900, including TypeScript client tests, stable install smoke (frozen sql/), and test (14–18). (claude-review is the known-broken check and did not run.)

Review findings

Backoff correctness — correct.

• Bounded: exactly one pollIntervalMs sleep is added on the two unfinished-batch paths (anyNackFailed, ack throw). No unbounded/exponential growth to reason about.
• Resets on success: the sleep lives only on the failure branches. A successful ack falls through with no delay, so a healthy consumer keeps draining at full speed — no regression to steady-state throughput.
• Abort stays responsive during the wait: sleep() is abort-aware (early-returns if already aborted; abort listener clears the timer and resolves), and the loop condition while (!signal?.aborted) re-checks immediately after the sleep, so shutdown latency during a backoff is bounded by the in-flight receive() only — unchanged from before.
• ack returning 0 (stale/double-ack) correctly stays warning-only with no sleep: that batch is finished, so the next poll gets a fresh batch, not the same one — no hot loop to defend against.

Double-processing semantics — inherent, correctly rate-limited. The skip-ack strategy means an unfinished batch is redelivered whole, so already-succeeded handlers in that batch re-run on redelivery (at-least-once). This PR does not change that contract; it only stops the redelivery from happening in a zero-delay tight loop. The updated comments describe this accurately (the old comment's "redelivered on the next poll" claim referenced a poll interval that did not exist on that path — now it does).

Bench-script interpolation — fixed correctly. producer_bench.ts now quotes the (self-generated, schema-qualified) table name from current_event_table() part-by-part via pg's escapeIdentifier before splicing. Not exploitable (dev-only, self-generated value), but it was the only non-parameterized query in the repo and is now safe. The paired <{ count: bigint }> generic fix is also correct — the pgque pool parses int8 (OID 20) to BigInt, so the old { count: string } + parseInt only worked by coercion.

TDD test — genuine red/green. The two new mock tests (pollInterval: 60_000, receive returns the same batch, nack/ack fail persistently) assert receive is called exactly once in the observation window. Verified locally: with the fix removed, the tests do not complete within a 120s timeout (the consumer hot-loops); with the fix they pass in ~312 ms each. TS style is consistent with the package.

Real testing (local, PG 16, devel build)

Fresh postgres:16 container, devel/sql/pgque.sql installed via psql -f.

bun run check                          # tsc x2 — clean
npx vitest run                         # no DSN: 35 passed | 50 skipped
PGQUE_TEST_DSN=... npx vitest run      # live devel DB: 8 files, 85 passed (0 skipped)

Red/green confirmation of the backoff fix (mock suite):

# fix removed:  vitest run -t "sleeps pollInterval"  -> hangs, killed at 120s (hot loop)
# fix restored: 2 passed  (312ms, 311ms)

Bench-script fix exercised live (hits the escapeIdentifier / BigInt verifyCount path):

PGQUE_TEST_DSN=... PGQUE_BENCH_REPEATS=1 bun src/producer_bench.ts
# table + CSV produced, verifyCount passed, no errors

Verdict

MERGE-READY. No blockers.

samorev Code Review Report

• PR: fix(ts): back off on nack/ack failure in consumer #289 - fix(ts): back off on nack/ack failure in consumer
• Author: @NikolayS
• AI-Assisted: Yes

This report supersedes the earlier samorev review --fetch comment on this PR. That comment was a CI/draft gate check only and performed no code analysis (it hardcoded zeros in every area). The results below come from a genuine per-area review of the diff, verified with a red/green test and a live devel-DB run on PostgreSQL 16.

No blocking issues. Reviewed for security, bugs, tests, guidelines, and documentation. One candidate was examined and filtered as an accepted design point (see below).

Result: PASSED

What was verified

• Backoff correctness: exactly one pollIntervalMs sleep is added on the two unfinished-batch paths (anyNackFailed, ack throw) — bounded, no exponential growth. The sleep lives only on failure branches, so a healthy consumer keeps draining at full speed (no steady-state regression). sleep() is abort-aware and the loop re-checks while (!signal?.aborted) immediately after, so shutdown latency during a backoff is bounded by the in-flight receive() only.
• ack returning 0 (stale/double-ack) correctly stays warning-only with no sleep — that batch is finished, so the next poll gets a fresh batch, not a hot loop.
• Bench-script hardening (in-scope fix): producer_bench.ts now quotes the self-generated, schema-qualified table name from current_event_table() part-by-part via escapeIdentifier before splicing — the only non-parameterized query in the repo, now safe (dev-only, self-generated value). The paired <{ count: bigint }> generic fix is correct (the pool parses int8/OID 20 to BigInt).
• Red/green TDD: the two new mock tests (pollInterval: 60_000, receive returns the same batch, nack/ack fail persistently) assert receive is called exactly once. Red (fix removed): tests hang, killed at 120s (hot loop). Green: pass in ~312 ms each.
• Full suite: bun run check clean; vitest run 35 passed / 50 skipped (no DSN); live devel DB 85 passed / 0 skipped; CI 16/16 green on the rebased head.

Filtered (candidate examined, excluded as a non-issue)

• Bugs — the skip-ack strategy redelivers an unfinished batch whole, so already-succeeded handlers in that batch re-run on redelivery (at-least-once). Examined and accepted: this is the inherent, pre-existing consumer contract; this PR does not change it, it only stops the redelivery from happening in a zero-delay tight loop. The updated comments describe it accurately. Not introduced here.

Summary

Note:

• Findings: High-confidence issues (8-10/10) - blocking or non-blocking per severity
• Potential: Medium-confidence issues (4-7/10) - review manually
• Filtered: Low-confidence issues (0-3/10) - excluded as likely false positives

provider=github
kind=pr
project=NikolayS/pgque
number=289
target=github:NikolayS/pgque#289
state=OPEN
draft=false
diff_lines=187
diff_added=125
diff_removed=6
diff_bytes=7093
comments_count=2
commits_count=2
ci_status=success
ci_summary=total=16 success=16 failure=0 pending=0 other=0
prompt=.claude/commands/review-mr.md
blocking=false
posted_by=gh
no_comment=false
live_posting=posted

samorev-assisted review (AI analysis by Tanya301/samorev)